Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

goverened gas pool spec #118

Open
wants to merge 10 commits into
base: l-monninger/governed-gas-pool
Choose a base branch
from

Conversation

l-monninger
Copy link
Collaborator

@l-monninger l-monninger commented Jan 3, 2025

Description

  • Creates governed_gas_pool.spec.move
  • Updated transaction_validation.spec.move

Type of Change

  • New feature
  • Bug fix
  • Breaking change
  • Performance improvement
  • Refactoring
  • Dependency update
  • Documentation update
  • Tests

Which Components or Systems Does This Change Impact?

  • Validator Node
  • Full Node (API, Indexer, etc.)
  • Move/Aptos Virtual Machine
  • Aptos Framework
  • Aptos CLI/SDK
  • Developer Infrastructure
  • Other (specify)

How Has This Been Tested?

Yes

  • aptos move prove -f governed_gas_pool && aptos move prove -f transaction_validation
  • aptos move test`

Key Areas to Review

IMPORTANT
When the feature COIN_TO_FUNGIBLE_ASSET_MIGRATION is enabled, this causes the spec fund in governed_gas_pool.spec.move to fail. So, I disabled it and it passes as expected.

I would advise us to disable this feature for mainnet as it seems strange to enable as we are not migrating to FA yet. However, if we do want to go to mainnet with this feature enabled, then I would imagine something is wrong with the spec.

NOTE
When trying to call governed_gas_pool_address() within the governed_gas_pool spec I get the following error

{
  "Error": "Move Prover failed: [internal] boogie exited with compilation errors:\n/Users/movses/mvmt/mvmt-aptos-core/aptos-move/framework/aptos-framework/boogie.bpl(12265,39): Error: use of undeclared function: $1_create_signer_$create_signer\n1 name resolution errors detected in /Users/movses/mvmt/mvmt-aptos-core/aptos-move/framework/aptos-framework/boogie.bpl\n"
}

@franck44 mentions this is a problem to do with versioning and configuration. I tried it with both aptos move prove and movement move prove, but it yields the same result. From the error message it seems that Boogie doesn't have a declaration for create_signer at runtime. So making ensures for resource accounts seems tricky.

In any case the changes offered here offer some value toward full verification.

spec aptos_framework::governed_gas_pool {
/// <high-level-req>
/// No.: 1
/// Requirement: The GovernedGasPool resource must exist at the aptos_framework address after initialization.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct.

/// Enforcement: Formally verified via [high-level-req-1](initialize).
///
/// No.: 2
/// Requirement: Only the aptos_framework address is allowed to initialize the GovernedGasPool.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct.

/// Enforcement: Formally verified via [high-level-req-2](initialize).
///
/// No.: 3
/// Requirement: Deposits into the GovernedGasPool must be reflected in the pool's balance.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say you want to prove above that initialization creates a resource account distinct from the aptos_framework address. Then deposits into the GovernedGasPool are reflected in that account's balance.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I try to to do ensures with anything resource account related I get the error noted down in the PR description.

/// Enforcement: Formally verified via [high-level-req-3](deposit), [high-level-req-3.1](deposit_from).
///
/// No.: 4
/// Requirement: Only the aptos_framework address can fund accounts from the GovernedGasPool.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct.

spec module {
/// [high-level-req-1]
/// The GovernedGasPool resource must exist at aptos_framework after initialization.
invariant exists<GovernedGasPool>(@aptos_framework);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The GovernGasPool exists for the aptos_framework cannot be an invariant. You would need an exists implies, i.e., ==>. One thing the GovernedGasPool existing could imply is that the resource account for the GovernedGasPool exists.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's passing though, I think because of

spec initialize(aptos_framework: &signer, delegation_pool_creation_seed: vector<u8>) {
        requires system_addresses::is_aptos_framework_address(signer::address_of(aptos_framework));
        /// [high-level-req-1]
        ensures exists<GovernedGasPool>(@aptos_framework);
    }
    ```

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can remove it if you we prefer. I suppose we would need to be doing an init_module for it to be an invariant.

@l-monninger
Copy link
Collaborator Author

@0xmovses do you have any of the transaction_fee.spec.move or transaction_validation.spec.move written out?

@0xmovses
Copy link
Collaborator

0xmovses commented Jan 7, 2025

@l-monninger Yes the changes for transaction_validation.spec.move are in the PR and passing. What you had commented, that logic is now running in the prover an passing.

@0xmovses 0xmovses self-assigned this Jan 7, 2025
@0xmovses 0xmovses changed the title 0xmovses/goverened gas pool spec goverened gas pool spec Jan 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants